Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use a single code server #26818

Open
wants to merge 1 commit into
base: dpeng817/use_code_server_start
Choose a base branch
from
Open

Use a single code server #26818

wants to merge 1 commit into from
+269 −142

Conversation

dpeng817
Copy link
Contributor

@dpeng817 dpeng817 commented Jan 3, 2025 •
edited
Loading

Summary & Motivation

Changes dagster dev to initialize a single code server instead of one per process (daemon, workspace).

As a result of this change, the grpc servers will no longer automatically start upon crash. To resolve this, introduce a "restart thread" which will automatically bring the proxy server back in the case of an unexpectedly dead proxy server.

How I Tested These Changes

Existing dagster dev tests, a new grpcserverregistry test where we kill the proxy server and ensure a new one is spun up.

Additionally tested manually and killed the underlying proxy server, ensured that it spun the proxy server back up without issue.

Changelog

  • dagster dev command now spins up a single code server per code location.

@dpeng817 dpeng817 mentioned this pull request Jan 3, 2025
Open
@dpeng817 Graphite App
Copy link
Contributor Author

dpeng817 commented Jan 3, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@dpeng817 dpeng817 mentioned this pull request Jan 3, 2025
Open
gibsondan
gibsondan reviewed Jan 6, 2025
View reviewed changes
Copy link
Member

@gibsondan gibsondan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think I put this on sean's original PR too - the main downside I see after this change is that if the proxy server process crashes, there is nothing that will detect that and restart it now. Before, if that happened, you could reload things from the webserver UI, and the daemon was restarting every 60 seconds anyway.

I'm not sure if that is a blocking concern or not, but I do know that there are some people who use 'dagster dev' in a non dev context, for better or worse, and that change in behavior could affect them. One possibility would be to make this new thing opt in or opt out in some fashion. Another would be to add something that monitors the proxy server process and restarts it automatically on the same port if that is detected.

@dpeng817 dpeng817 force-pushed the dpeng817/use_code_server_start branch from 21a03b4 to 9b7602e Compare January 7, 2025 05:34
@dpeng817 dpeng817 force-pushed the dpeng817/single_code_server branch from c761541 to e71f7f6 Compare January 7, 2025 05:34
@dpeng817 dpeng817 force-pushed the dpeng817/use_code_server_start branch from 9b7602e to 3e118fd Compare January 7, 2025 05:35
@dpeng817 dpeng817 force-pushed the dpeng817/single_code_server branch from e71f7f6 to 1bf450d Compare January 7, 2025 05:35
@dpeng817 dpeng817 marked this pull request as ready for review January 7, 2025 05:35
@dpeng817 Graphite App
Copy link
Contributor Author

dpeng817 commented Jan 7, 2025

Restarting automatically on the same port seems like the most principled approach.

@dpeng817 dpeng817 force-pushed the dpeng817/single_code_server branch from 1bf450d to a31c4a9 Compare January 7, 2025 06:29
@dpeng817 dpeng817 force-pushed the dpeng817/use_code_server_start branch from 3e118fd to 982a6d4 Compare January 7, 2025 21:55
@dpeng817 dpeng817 force-pushed the dpeng817/single_code_server branch from a31c4a9 to 43a6d66 Compare January 7, 2025 21:55
@gibsondan gibsondan self-requested a review January 7, 2025 22:36
@dpeng817 dpeng817 force-pushed the dpeng817/use_code_server_start branch from 982a6d4 to dd0a28f Compare January 7, 2025 22:51
@dpeng817 dpeng817 force-pushed the dpeng817/single_code_server branch 3 times, most recently from af1816d to 0bd9734 Compare January 7, 2025 23:59
@dpeng817 dpeng817 force-pushed the dpeng817/use_code_server_start branch from dd0a28f to 71a4627 Compare January 8, 2025 00:04
@dpeng817 dpeng817 force-pushed the dpeng817/single_code_server branch from 0bd9734 to 4571581 Compare January 8, 2025 00:04
@dpeng817 dpeng817 requested a review from gibsondan January 8, 2025 00:58
@dpeng817 dpeng817 force-pushed the dpeng817/single_code_server branch from 4571581 to 411f4ce Compare January 8, 2025 01:41
gibsondan
gibsondan previously requested changes Jan 8, 2025
View reviewed changes
Copy link
Member

@gibsondan gibsondan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is very close - a few things inline, the other thing is figuring out how to keep the display metadata so you can still see the python module/package/etc. even though they are now sourced from a grpc port/socket

python_modules/dagster/dagster/_cli/dev.py Outdated Show resolved Hide resolved
python_modules/dagster/dagster/_cli/dev.py Outdated Show resolved Hide resolved
python_modules/dagster/dagster/_cli/dev.py Outdated Show resolved Hide resolved
python_modules/dagster/dagster/_grpc/server.py Show resolved Hide resolved
python_modules/dagster/dagster/_grpc/server.py Outdated Show resolved Hide resolved
@dpeng817 dpeng817 force-pushed the dpeng817/single_code_server branch 2 times, most recently from 6964310 to f0caf87 Compare January 8, 2025 18:35
@dpeng817 dpeng817 requested a review from gibsondan January 8, 2025 18:36
@dpeng817 dpeng817 force-pushed the dpeng817/use_code_server_start branch from 71a4627 to 1cd13dc Compare January 8, 2025 20:13
@dpeng817 dpeng817 force-pushed the dpeng817/single_code_server branch from f0caf87 to 0b9624f Compare January 8, 2025 20:14
@dpeng817 dpeng817 force-pushed the dpeng817/single_code_server branch from 0b9624f to 99884f2 Compare January 8, 2025 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibsondan gibsondan Awaiting requested review from gibsondan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dpeng817 @gibsondan